Skip to content

Dealocate unused memory when queuing transactions #6115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Dealocate unused memory when queuing transactions #6115

wants to merge 2 commits into from
+3 −0

Conversation

@Amxx
Copy link
Collaborator

@Amxx Amxx commented Nov 14, 2025

Added memory management for transaction queuing in GovernorTimelockCompound.

Fixes #????

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@Amxx
Dealocate unused memory when queuing transactions
0aed511 
Added memory management for transaction queuing in GovernorTimelockCompound.
@Amxx Amxx requested a review from a team as a code owner November 14, 2025 15:20
@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 0333638

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Amxx
Amxx commented Nov 14, 2025
View reviewed changes
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025 •
edited
Loading

Walkthrough

The GovernorTimelockCompound._queueOperations function is updated with memory management modifications. The change imports Memory.sol and implements pointer-based memory operations: it retrieves a free memory pointer before the loop, and resets the pointer after each duplicate-queue check to deallocate memory reserved by abi.encode calls. No public function signatures or external interfaces are altered.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: adding memory deallocation when queuing transactions in GovernorTimelockCompound.
Description check ✅ Passed The description relates to the changeset by mentioning memory management for transaction queuing in GovernorTimelockCompound, though it lacks implementation details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch Amxx-patch-1
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6308fdc and 0333638.

📒 Files selected for processing (1)
  • contracts/governance/extensions/GovernorTimelockCompound.sol (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Redirect rules - solidity-contracts
  • GitHub Check: Header rules - solidity-contracts
  • GitHub Check: Pages changed - solidity-contracts
  • GitHub Check: coverage
  • GitHub Check: slither
  • GitHub Check: tests-foundry
  • GitHub Check: tests-upgradeable
  • GitHub Check: tests
🔇 Additional comments (1)
contracts/governance/extensions/GovernorTimelockCompound.sol (1)

9-9: Memory library correctly implemented; pattern verified across codebase.

The Memory library at contracts/utils/Memory.sol is properly implemented with correct getFreeMemoryPointer() and setFreeMemoryPointer() functions marked with assembly ("memory-safe"). The usage pattern in GovernorTimelockCompound.sol is consistent with ERC4626.sol: it saves the free memory pointer before the loop, reuses the same memory region for each iteration's abi.encode call, and deallocates after consuming the encoded data. This is an efficient pattern that prevents memory bloat during batch operations. No conflicts with queueTransaction or side effects detected.

Comment @coderabbitai help to get the list of available commands and usage tips.

ernestognw
ernestognw reviewed Nov 14, 2025
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is more costly than the initial version according to the gas report

$GovernorTimelockCompoundMock queue(address[],uint256[],bytes[],bytes32) 116561 +28 +0.03% ❌ 151116 +10 +0.01% ❌ 120111 +25 +0.03% ❌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ernestognw ernestognw ernestognw left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Amxx @ernestognw